-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MDEV-35847 mariadb client --wait flag broken #3763
base: 10.5
Are you sure you want to change the base?
Conversation
kind of risky to enable it after it was disabled for 23 years. I'd rather keep the status quo and remove it completely |
although it works in mariadb-admin |
9d1b45d
to
fcd6600
Compare
I'm curious why the last committer intended to delete this option because the rest of the logic for it is in place. It would've been trivial to assume |
I'm not sure the last committer has actually intended that. It's quite possibly could've been a mistake. I don't have a strong opinion about it. It was a dead code for 23 years, so clearly nobody cares whether it works or not and it could be safely deleted. On the other hand, |
I'm biased (since I wrote the patch 😇 ) but really it's up to you. I'm happy to finish removing the flag, but like you said it would be nice for feature symmetry between the two clients (and maybe someone is using it for the admin client). |
Wire up the --wait flag in the mariadb client.
fcd6600
to
7113727
Compare
you wrote you need it for some tooling, so it's plausible that someone else might need it too. let's keep it |
OK 😄 once you approve the PR then I'll merge. The target branch is 10.5, let me know if you'd rather it go somewhere else instead (11.8?). |
@@ -1732,7 +1732,7 @@ static struct my_option my_long_options[] = | |||
GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, | |||
{"vertical", 'E', "Print the output of a query (rows) vertically.", | |||
&vertical, &vertical, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, | |||
{"wait", 'w', "Wait and retry if connection is down.", 0, 0, 0, GET_NO_ARG, | |||
{"wait", 'w', "Wait and retry if connection is down.", &wait_flag, 0, 0, GET_BOOL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's not how it used to work. see commit c9d0428 (wait time was configurable, retrying forever)
and it's not how -w
works in mariadb-admin
(wait time is hard-coded, 5 seconds, number of retries is configurable)
For consistency, I suppose, it should do what mariadb-admin
does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll make the changes 👍
Wire up the --wait flag in the mariadb client.